-
Notifications
You must be signed in to change notification settings - Fork 97
Apply some async hooks changes from upstream #87
Conversation
@@ -150,38 +150,39 @@ static void DestroyIdsCb(uv_timer_t* handle) { | |||
TryCatch try_catch(env->isolate()); | |||
|
|||
do { | |||
std::vector<double> destroy_ids_list; | |||
destroy_ids_list.swap(*env->destroy_ids_list()); | |||
std::vector<double> destroy_async_id_list; | |||
if (!env->can_call_into_js()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(conflict no 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the swapping of can_call_into_js
check and the list swap intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu Not really intentional, but I don’t think it matters – that there is a similar check in PushBackDestroyAsyncId()
means we’re okay with dropping destroy
callbacks if can_call_into_js()
is false.
If we were not okay with that, I think this change would make it More Correct™, because right now we would silently drop the destroy id queue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, just making sure this wasn't overlooked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thanks for pointing to it :)
} | ||
|
||
|
||
static void PushBackDestroyId(Environment* env, double id) { | ||
static void PushBackDestroyAsyncId(Environment* env, double id) { | ||
if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) | ||
return; | ||
|
||
if (!env->can_call_into_js()) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(conflict no 2)
PR-URL: ayojs#87 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Original-PR-URL: nodejs/node#15569 Original-Reviewed-By: Refael Ackermann <refack@gmail.com> Original-Reviewed-By: Anna Henningsen <anna@addaleax.net> Original-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Original-Reviewed-By: James M Snell <jasnell@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
If bootstrap throws and if ids are added to the async id stack and if the exception wasn't handled by the fatal exception handler then the AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack check to fail. Causing the application to crash. So clear the async id stack manually. This is only possible if the user: 1) manually calls MakeCallback() or 2) uses async await in the top level. Which will cause _tickCallback() to fire before bootstrap finishes executing. The following example shows how the application can fail due to exceeding the maximum call stack while using async await: async function fn() { fn(); throw new Error(); } (async function() { await fn(); })(); If this occurs during bootstrap then the application will pring the following warning a number of times then exit with a status of 0: (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error Here's the same recursive call done after enabling a new AsyncHook() the following will print instead of the above warning and exit with a non-zero code (currently it's 7 because of how node::FatalException assigns error codes based on where the failure happened): script.js:25 async function fn() { ^ RangeError: Maximum call stack size exceeded at <anonymous> at fn (script.js:25:18) at fn (script.js:26:3) .... This has to do with how Promises lazily enable PromiseHook if an AsyncHook() is enabled. Whether these need to be made uniform is outside the scope of this commit Fixes: nodejs/node#15448 PR-URL: nodejs/node#15553 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
9a16a7b
to
e82107f
Compare
All of the conflicts were in the first commit, and only with
env->can_call_into_js()
calls, so this should not be a big deal.